-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix: cip64 regressions #6
Conversation
fb17e68
to
638714f
Compare
size-limit report 📦
|
3835187
to
3e41cd9
Compare
3e41cd9
to
ed70912
Compare
bd6e922
to
86e52cb
Compare
const snapshot = | ||
'"0x7cf8840182031184773594008504a817c80082520894765de816845861e75a25fca122bb6898b8b1282a940f16e9b0d03470827a95cdfd0cb8a8a3b46969b904808080c001a062bee7f81cccd1f430b4b66ec5a23737d6fbee9965e63ac582d09f63aef32bdca05e75bd3ef63f2c0f6fd0a87e3f8d4809a38ac955b7d97fd4af1bd2c882999d5c"' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Note-to-self) Would love to better understand how snapshots are generated and pasted here (practically). I read about Snapshot testing here: https://jestjs.io/docs/snapshot-testing#inline-snapshots.
It sounds like these are generated "automagically":
First, you write a test, calling
.toMatchInlineSnapshot()
with no arguments
The next time you run Jest, tree will be evaluated, and a snapshot will be written as an argument to toMatchInlineSnapshot
By default, Jest handles the writing of snapshots into your source code. However, if you're using prettier in your project, Jest will detect this and delegate the work to prettier instead (including honoring your configuration).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
while it doesn't differ much on the snapshot front, viem is using vitest for testing rather than jest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: (not related to changes in this PR)
Based on this recent Slack convo with @mcortesi, transaction receipts do not include feeCurrency
, gatewayFee
, or gatewayFeeRecipient
fields, which means they are undefined
in every transaction receipt object (by default). We can thus look into removing them from the expected and actual transaction receipt (in tests), because they will always trivially be equal.
I suggest we fix this in a separate PR to keep this one clean and get the regression fix merged as soon as possible.
describe('transactionReceipt', () => {
test('formatter', () => {
const { transactionReceipt } = celo.formatters!
expect(
transactionReceipt.format({
blockHash:
'0x89644bbd5c8d682a2e9611170e6c1f02573d866d286f006cbf517eec7254ec2d',
blockNumber: '0x1',
contractAddress: '0xa152f8bb749c55e9943a3a0a3111d18ee2b3f94e',
cumulativeGasUsed: '0x2',
effectiveGasPrice: '0x3',
- feeCurrency: null,
from: '0xa152f8bb749c55e9943a3a0a3111d18ee2b3f94e',
gasUsed: '0x4',
- gatewayFee: null,
- gatewayFeeRecipient: null,
logs: [],
to: '0x15d4c048f83bd7e37d49ea4c83a07267ec4203da',
status: '0x0',
type: '0x0',
}),
).toMatchInlineSnapshot(`
{
"blockHash": "0x89644bbd5c8d682a2e9611170e6c1f02573d866d286f006cbf517eec7254ec2d",
"blockNumber": 1n,
"contractAddress": "0xa152f8bb749c55e9943a3a0a3111d18ee2b3f94e",
"cumulativeGasUsed": 2n,
"effectiveGasPrice": 3n,
- "feeCurrency": null,
"from": "0xa152f8bb749c55e9943a3a0a3111d18ee2b3f94e",
"gasUsed": 4n,
- "gatewayFee": null,
- "gatewayFeeRecipient": null,
"logs": [],
"status": "reverted",
"to": "0x15d4c048f83bd7e37d49ea4c83a07267ec4203da",
"transactionIndex": null,
"type": "legacy",
}
`)
src/chains/celo/formatters.ts
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: (not related to changes in this PR)
Based on this recent Slack convo with @mcortesi, transaction receipts do not include feeCurrency
, gatewayFee
, or gatewayFeeRecipient
fields, which means they are undefined
in every transaction receipt object (by default). We can thus look into removing them from here
transactionReceipt: /*#__PURE__*/ defineTransactionReceipt({
format(
args: CeloRpcTransactionReceiptOverrides,
): CeloTransactionReceiptOverrides {
return {
- feeCurrency: args.feeCurrency,
- gatewayFee: args.gatewayFee ? hexToBigInt(args.gatewayFee) : null,
- gatewayFeeRecipient: args.gatewayFeeRecipient,
}
},
}),
I suggest we fix this in a separate PR to keep this one clean and get the regression fix merged as soon as possible.
|
||
export function isEmpty( | ||
value: string | undefined | number | BigInt, | ||
): value is undefined { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(note-to-self) Neat! Learnt about "user-defined type guards" e.g. value is undefined
.
return ( | ||
isEIP1559(transaction) && | ||
isPresent(transaction.feeCurrency) && | ||
isEmpty(transaction.gatewayFee) && | ||
isEmpty(transaction.gatewayFeeRecipient) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is neat, makes it a lot more readable. Thanks for refactoring this!
86e52cb
to
bc31c3f
Compare
) { | ||
throw new BaseError( | ||
'`gatewayFee` and `gatewayFeeRecipient` must be provided together.', | ||
) | ||
} | ||
|
||
if (feeCurrency && !feeCurrency?.startsWith('0x')) { | ||
if (isPresent(feeCurrency) && !isAddress(feeCurrency)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This reads so much better with isPresent
and isAddress
, thanks for refactoring this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not the strongest approval since, I haven't worked in viem
before, but this looks good to me. Love the refactored use of utils
.
Note for reviewers, we paired with @0xarthurxyz to run the build locally and try if the failing transactions were fixed, and they were. |
I see viem defines the EIP1559 TX like
note the My gut is they should be the same
|
* Fix: cip64 regressions (#6) * fix: refactor cip64 to be more robust * fix: types * fix: signTransaction tests * refactor: PR feedback * chore: add changeset * test: add celo/utils test coverage (#8) * CELO utils test coverage * test: generate new address for each test --------- Co-authored-by: Nicolas Brugneaux <[email protected]> * chore: update src/chains/celo/utils.ts Co-authored-by: jxom <[email protected]> * Update yellow-eggs-compare.md --------- Co-authored-by: Leszek Stachowski <[email protected]> Co-authored-by: jxom <[email protected]>
This PR aims to fix the issues reported by @0xarthurxyz in celo-org/txtypes#1
This refactors the different checks for CIP42 and CIP64 by using more robust mechanism than truthy/falsy values.
It also removes the tests that added
type: 'cipxyz'
unnecessarily and by doing so forcing the tx to be correct even though the transaction-type inference not detect it as such. (eg: a cip42 without gatewayFee).It also removes the brittle tests hardcoding the raw serialized string (keeps it in a couple for sanity) but uses the parser to self-check itself.